-
-
Notifications
You must be signed in to change notification settings - Fork 218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow WebUser invites to be editable before the invited user accepts it #35538
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't able to do a super thorough review, but it looks good save for a couple nitpicks
corehq/apps/registration/forms.py
Outdated
|
||
self.helper = FormHelper() | ||
self.helper.form_method = 'POST' | ||
self.helper.form_class = 'form-horizontal form-ko-validation' | ||
|
||
self.helper.label_class = 'col-sm-3 col-md-2' | ||
self.helper.field_class = 'col-sm-9 col-md-8 col-lg-6' | ||
|
||
save_button_text = "Send Invite" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this and the alternate text should be marked for translation
corehq/apps/registration/forms.py
Outdated
@@ -575,8 +584,7 @@ def __init__(self, data=None, is_add_user=None, | |||
), | |||
hqcrispy.FormActions( | |||
twbscrispy.StrictButton( | |||
(gettext("Add User") if is_add_user | |||
else gettext("Send Invite")), | |||
(gettext("Add User") if is_add_user else gettext(save_button_text)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
passing a variable to gettext
will only translate that var if it's already marked for translation elsewhere, such as with gettext_noop
. Probably most straightforward to just move the gettext
call to where those strings are defined in this case.
corehq/apps/users/views/__init__.py
Outdated
if invitation and invitation.email in pending_invites: | ||
pending_invites.remove(invitation.email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow, what's going on here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The invite form will normally throw an error if you try to send an invite to an email that has an invite pending/already belongs to the domain. Since the edit form reuses that same form, I need to take out the email belonging to the invite that's being edited so it can submit/save properly.
corehq/apps/users/views/__init__.py
Outdated
@@ -1144,7 +1144,9 @@ def invite_web_user_form(self): | |||
if invitation: | |||
assigned_location = invitation.assigned_locations.filter() | |||
assigned_location_ids = [loc.location_id for loc in assigned_location] | |||
primary_location_id = invitation.primary_location.location_id | |||
primary_location_id = None | |||
if assigned_location_ids: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this check primary_location
instead of assigned_location_ids
? It should work the same in either case, just seems clearer
…t_field_page_context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, good work on this! And thank you for responding to my million comments
@@ -594,7 +606,7 @@ def clean_email(self): | |||
email = self.cleaned_data['email'].strip() | |||
|
|||
from corehq.apps.registration.validation import AdminInvitesUserFormValidator | |||
error = AdminInvitesUserFormValidator.validate_email(self.domain, email) | |||
error = AdminInvitesUserFormValidator.validate_email(self.domain, email, bool(self.invite)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to skip all this validation if the field is set as read only? And then add a validation that the email didn't change like
if self.fields['email'].widget.attrs.get('readonly', False):
if email != self.invite.email:
raise forms.ValidationError("The email field is read-only and cannot be changed.")
```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what I have currently is simpler
Product Description
Users will now have the ability to edit web user invites before the invited user has accepted it. They'll be able to change any field that was visible to them when initially making the invitation (except the email field - if a user wants to change who the invite is for, they will have to delete the existing invite and create a new one). This edit page will be accessible through a new 'Edit' button that'll appear next to every pending invite.
When entering the form, the user will see that they are now in the 'Edit Web User Invite' page on the sidebar menu and the submit button will show 'Update Invite'. Editing the invite will not send a new invite email.
This change is compatible with the web_user_invite_additional_fields feature flag, which adds additional user defined fields to the invite form.
Technical Summary
Jira Ticket
This new edit form reuses the original create form but checks first to see if an invitation id is in the request. If there is one, it'll pre-populate the form with previously submitted input wherever is applicable.
Feature Flag
Doesn't strictly use it but is compatible with web_user_invite_additional_fields.
Safety Assurance
Safety story
Tested locally and on staging. The blast radius is limited to the user/invitation creation operations, including mobile worker creation and domain requests (both areas I've asked QA to cover).
QA Plan
QA ticket
Rollback instructions
Labels & Review